Add plugin infra for adding new dashboard tabs/cards #1742

rawagner · 2019-06-18T11:50:15Z

openshift-ci-robot · 2019-06-18T11:50:31Z

Hi @rawagner. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

spadgett · 2019-06-18T15:50:32Z

/approve
/assign @vojtechszocs

spadgett · 2019-06-18T15:59:09Z

/ok-to-test

rawagner · 2019-06-18T16:07:53Z

After discussion with @vojtechszocs I changed this a bit - adding tabs and cards is a separate extension. I also improved types a bit.

frontend/packages/console-plugin-sdk/src/typings/dashboards.ts

vojtechszocs · 2019-06-18T18:35:09Z

frontend/packages/console-plugin-sdk/src/typings/dashboards.ts

+    position: GridPosition;
+
+    /** The card component to render */
+    component: React.ComponentType<any>;


cc @spadgett @christianvogt

Directly referencing a React component from within a plugin's entry module will make it part of the Console initial chunk, which is already too big.

The default strategy for referencing plugin-contributed React components should be

Promise<TypeOfComponent>

and using either

() => import('./path/to/module' /* webpackChunkName: "chunk-name" */).then(m => m.MyComponent)

or

async () => (await import('./path/to/module' /* webpackChunkName: "chunk-name" */)).MyComponent

to provide those components.

Please add the following into typings/index.ts:

type LazyLoader<T> = () => Promise<React.ComponentType<T>>;

and use it in both typings/pages.ts and typings/dashboards.ts.

I cant move LazyLoader type to typings/index.ts because that results in a circular dependency. Moved it to typings/types.ts.

vojtechszocs · 2019-06-18T18:39:20Z

frontend/public/components/app-contents.tsx

@@ -98,7 +98,7 @@ const AppContents = withRouter(React.memo(() => (

          <Route path={['/all-namespaces', '/ns/:ns']} component={RedirectComponent} />

-          <LazyRoute path="/dashboards" exact loader={() => import('./dashboards-page/dashboards' /* webpackChunkName: "dashboards" */).then(m => m.DashboardsPage)} />
+          <LazyRoute path="/dashboards" loader={() => import('./dashboards-page/dashboards' /* webpackChunkName: "dashboards" */).then(m => m.DashboardsPage)} />


FYI @spadgett plugins can add new tabs into existing Dashboards page, with tab ID reflected into the URL.

vojtechszocs · 2019-06-18T18:47:29Z

frontend/public/components/dashboards-page/dashboards.tsx

  {
    href: '',
    name: 'Overview',
    component: OverviewDashboard,
  },
 ];

+const getCardsOnPosition = (cards: DashboardsCard[], position: GridPosition): React.ComponentType<any>[] =>


To render lazy (async) components, you can use Promise-based AsyncComponent.

Using AsyncComponent now

vojtechszocs · 2019-06-18T18:49:36Z

My only concern here is to ensure that we load custom components lazily instead of referencing them directly, please see my inline comments for details.

rawagner · 2019-06-19T07:11:12Z

My only concern here is to ensure that we load custom components lazily instead of referencing them directly, please see my inline comments for details.

Thanks @vojtechszocs for review. I addressed all issues, can you take a look again ?

vojtechszocs · 2019-06-20T17:18:27Z

frontend/public/components/dashboards-page/dashboards.tsx

+  return plugins.registry.getDashboardsTabs().map(tab => {
+    const tabCards = cards.filter(c => c.properties.tab === tab.properties.id);
+    return {
+      href: tab.properties.id,


We should also write an extension check that ensures tab IDs are unique (and non-empty as well), I'll put it on my task list.

In general, every time we add new extension types, we should also consider writing the corresponding extension checks to guard against potential conflicts.

frontend/public/components/dashboards-page/dashboards.tsx

rawagner · 2019-06-21T11:29:58Z

@spadgett can you take a look ?

cloudbehl · 2019-06-24T12:15:34Z

@spadgett @rawagner Blocked on this PR to be merged. Can we merge this PR?

jelkosz · 2019-06-24T12:17:14Z

@cloudbehl you should not really be blocked, the API should not change so it is safe to base your work on this already

cloudbehl · 2019-06-24T12:33:28Z

@jelkosz ok, I was waiting for the PR to be merged. I have raised a dependent PR[1] for now once merged I will rebase it. Thanks

[1]. #1803

vojtechszocs · 2019-06-26T11:38:13Z

/lgtm

openshift-ci-robot · 2019-06-26T11:38:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rawagner, spadgett, vojtechszocs

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~frontend/OWNERS~~ [spadgett]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

rawagner · 2019-06-26T12:54:13Z

/retest

openshift-ci-robot requested review from christianvogt and jhadvig June 18, 2019 11:50

openshift-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 18, 2019

rawagner mentioned this pull request Jun 18, 2019

Add plugin infra for adding new dashboard tabs/cards #1740

Closed

openshift-ci-robot assigned vojtechszocs Jun 18, 2019

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 18, 2019

openshift-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 18, 2019

spadgett added this to the v4.2 milestone Jun 18, 2019

rawagner force-pushed the dashboards-page branch from 41bceca to 723918a Compare June 18, 2019 16:05

vojtechszocs reviewed Jun 18, 2019

View reviewed changes

rawagner force-pushed the dashboards-page branch from 723918a to ef90913 Compare June 19, 2019 07:06

vojtechszocs reviewed Jun 20, 2019

View reviewed changes

vojtechszocs suggested changes Jun 20, 2019

View reviewed changes

frontend/public/components/dashboards-page/dashboards.tsx Outdated Show resolved Hide resolved

frontend/public/components/dashboards-page/dashboards.tsx Outdated Show resolved Hide resolved

vojtechszocs mentioned this pull request Jun 20, 2019

Add capacity card #1723

Merged

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 20, 2019

vojtechszocs approved these changes Jun 20, 2019

View reviewed changes

Add plugin infra for adding new dashboard tabs/cards

e29751c

rawagner force-pushed the dashboards-page branch from aa9507b to e29751c Compare June 20, 2019 18:47

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 20, 2019

cloudbehl mentioned this pull request Jun 24, 2019

Health and Details card for ceph dashboard #1803

Merged

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 26, 2019

openshift-merge-robot merged commit 3034dac into openshift:master Jun 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add plugin infra for adding new dashboard tabs/cards #1742

Add plugin infra for adding new dashboard tabs/cards #1742

rawagner commented Jun 18, 2019 •

edited

openshift-ci-robot commented Jun 18, 2019

spadgett commented Jun 18, 2019

spadgett commented Jun 18, 2019

rawagner commented Jun 18, 2019

vojtechszocs Jun 18, 2019

rawagner Jun 19, 2019

vojtechszocs Jun 18, 2019

vojtechszocs Jun 18, 2019

rawagner Jun 19, 2019

vojtechszocs commented Jun 18, 2019

rawagner commented Jun 19, 2019

vojtechszocs Jun 20, 2019 •

edited

rawagner commented Jun 21, 2019

cloudbehl commented Jun 24, 2019

jelkosz commented Jun 24, 2019

cloudbehl commented Jun 24, 2019

vojtechszocs commented Jun 26, 2019

openshift-ci-robot commented Jun 26, 2019

rawagner commented Jun 26, 2019

Add plugin infra for adding new dashboard tabs/cards #1742

Add plugin infra for adding new dashboard tabs/cards #1742

Conversation

rawagner commented Jun 18, 2019 • edited

openshift-ci-robot commented Jun 18, 2019

spadgett commented Jun 18, 2019

spadgett commented Jun 18, 2019

rawagner commented Jun 18, 2019

vojtechszocs Jun 18, 2019

Choose a reason for hiding this comment

rawagner Jun 19, 2019

Choose a reason for hiding this comment

vojtechszocs Jun 18, 2019

Choose a reason for hiding this comment

vojtechszocs Jun 18, 2019

Choose a reason for hiding this comment

rawagner Jun 19, 2019

Choose a reason for hiding this comment

vojtechszocs commented Jun 18, 2019

rawagner commented Jun 19, 2019

vojtechszocs Jun 20, 2019 • edited

Choose a reason for hiding this comment

rawagner commented Jun 21, 2019

cloudbehl commented Jun 24, 2019

jelkosz commented Jun 24, 2019

cloudbehl commented Jun 24, 2019

vojtechszocs commented Jun 26, 2019

openshift-ci-robot commented Jun 26, 2019

rawagner commented Jun 26, 2019

rawagner commented Jun 18, 2019 •

edited

vojtechszocs Jun 20, 2019 •

edited